tests: Require run-fail ui tests to have an exit code (SIGABRT not ok)
#143002
+190
−99
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
rustbot has assigned @petrochenkov. Use |
rustbot
added
A-compiletest
|
Some changes occurred in src/tools/compiletest cc @jieyouxu |
Enselic
added
S-waiting-on-author
rustbot
added
the
T-infra
This comment has been minimized.
This comment has been minimized.
|
why not simply also would accept |
|
@bors2 delegate=try |
|
@Enselic can now perform try builds on this pull request |
Enselic
force-pushed
the
tests-ui-run-fail-exit-vs-signal
branch
from
6ad1973 to
17be091
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
@bors2 try jobs=x86_64-msvc-1,x86_64-msvc-2 |
rust-bors bot
added a commit
that referenced
this pull request
Jun 25, 2025
…try> tests: Require `run-fail` ui tests to have an exit code (`SIGABRT` not ok) Normally a `run-fail` ui test shall not be terminated by a signal like `SIGABRT`. So begin having that as a hard requirement. Some of our current tests do terminate by a signal however. Introduce and use `run-fail-without-exit-code` for those tests. This adds further (cc #142304, #142886) protection against the regression in #123733 since that bug also manifested as `SIGABRT` in `tests/ui/panics/panic-main.rs` (shown as `Aborted (core dumped)` in the logs attached to that issue, and I have also been able to reproduce this locally). ### TODO - [ ] what about on Windows? - [ ] also update docs at https://rustc-dev-guide.rust-lang.org/tests/directives.html#controlling-outcome-expectations - [ ] clean up the code ### Zulip discussion See https://rust-lang.zulipchat.com/#narrow/channel/122651-general/topic/compiletest.3A.20terminate.20by.20signal.20vs.20exit.20with.20error/with/525611235 try-job: x86_64-msvc-1 try-job: x86_64-msvc-2
This comment was marked as resolved.
This comment was marked as resolved.
Enselic
force-pushed
the
tests-ui-run-fail-exit-vs-signal
branch
from
17be091 to
ad4e082
Compare
|
An only-windows test that was run-fail now must be run-crash. Fixed now. Trying again: @bors2 try jobs=x86_64-msvc-1,x86_64-msvc-2 |
rust-bors bot
added a commit
that referenced
this pull request
Jun 25, 2025
…try> tests: Require `run-fail` ui tests to have an exit code (`SIGABRT` not ok) Normally a `run-fail` ui test shall not be terminated by a signal like `SIGABRT`. So begin having that as a hard requirement. Some of our current tests do terminate by a signal however. Introduce and use `run-fail-without-exit-code` for those tests. This adds further (cc #142304, #142886) protection against the regression in #123733 since that bug also manifested as `SIGABRT` in `tests/ui/panics/panic-main.rs` (shown as `Aborted (core dumped)` in the logs attached to that issue, and I have also been able to reproduce this locally). ### TODO - [ ] **Q:** what about on Windows? **A:** we'll treat any exit code outside of 1 - 127 as "crashed", and we'll do the same on unix. - [ ] also update docs at https://rustc-dev-guide.rust-lang.org/tests/directives.html#controlling-outcome-expectations - [ ] clean up the code - [ ] test all permutations of actual vs expected ### Zulip discussion See https://rust-lang.zulipchat.com/#narrow/channel/122651-general/topic/compiletest.3A.20terminate.20by.20signal.20vs.20exit.20with.20error/with/525611235 try-job: x86_64-msvc-1 try-job: x86_64-msvc-2
This comment was marked as resolved.
This comment was marked as resolved.
Enselic
force-pushed
the
tests-ui-run-fail-exit-vs-signal
branch
from
ad4e082 to
2499dac
Compare
Enselic
force-pushed
the
tests-ui-run-fail-exit-vs-signal
branch
2 times, most recently
from
c19e167 to
7914e18
Compare
|
The problems found by bors in the approved commit 4573c84 should be fixed now. Changes in the current commit compared to that commit:
Since the @rustbot ready Updated examples of failure messages:How exit code 0 looks like for a
|
rustbot
added
S-waiting-on-review
This comment was marked as resolved.
This comment was marked as resolved.
Enselic
force-pushed
the
tests-ui-run-fail-exit-vs-signal
branch
from
7914e18 to
5334e8a
Compare
This comment has been minimized.
This comment has been minimized.
Enselic
force-pushed
the
tests-ui-run-fail-exit-vs-signal
branch
from
5334e8a to
7f2f79f
Compare
petrochenkov
reviewed
Jul 3, 2025
|
r=me with #143002 (comment) addressed. |
rustbot
removed
the
S-waiting-on-review
|
✌️ @Enselic, you can now approve this pull request! If @petrochenkov told you to " |
rustbot
added
the
S-waiting-on-author
…t ok) And introduce two new directives for ui tests: * `run-crash` * `run-fail-or-crash` Normally a `run-fail` ui test like tests that panic shall not be terminated by a signal like `SIGABRT`. So begin having that as a hard requirement. Some of our current tests do terminate by a signal/crash however. Introduce and use `run-crash` for those tests. Note that Windows crashes are not handled by signals but by certain high bits set on the process exit code. Example exit code for crash on Windows: `0xc000001d`. Because of this, we define "crash" on all platforms as "not exit with success and not exit with a regular failure code in the range 1..=127". Some tests behave differently on different targets: * Targets without unwind support will abort (crash) instead of exit with failure code 101 after panicking. As a special case, allow crashes for `run-fail` tests for such targets. * Different sanitizer implementations handle detected memory problems differently. Some abort (crash) the process while others exit with failure code 1. Introduce and use `run-fail-or-crash` for such tests.
Enselic
force-pushed
the
tests-ui-run-fail-exit-vs-signal
branch
from
7f2f79f to
6950daf
Compare
bors
added
S-waiting-on-bors
bors
added a commit
that referenced
this pull request
Jul 4, 2025
…etrochenkov tests: Require `run-fail` ui tests to have an exit code (`SIGABRT` not ok) And introduce two new directives for ui tests: * `run-crash` * `run-fail-or-crash` Normally a `run-fail` ui test like tests that panic shall not be terminated by a signal like `SIGABRT`. So begin having that as a hard requirement. Some of our current tests do terminate by a signal/crash however. Introduce and use `run-crash` for those tests. Note that Windows crashes are not handled by signals but by certain high bits set on the process exit code. Example exit code for crash on Windows: `0xc000001d` (`STATUS_ILLEGAL_INSTRUCTION`). Because of this, we define "crash" on all platforms as "not exit with success and not exit with a regular failure code in the range 1..=127". Some tests behave differently on different targets: * Targets without unwind support will abort (crash) instead of exit with failure code 101 after panicking. As a special case, allow crashes for `run-fail` tests for such targets. * Different sanitizer implementations handle detected memory problems differently. Some abort (crash) the process while others exit with failure code 1. Introduce and use `run-fail-or-crash` for such tests. This adds further (cc #142304, #142886) protection against the regression in #123733 since that bug also manifested as `SIGABRT` in `tests/ui/panics/panic-main.rs` (shown as `Aborted (core dumped)` in the logs attached to that issue, and I have also been able to reproduce this locally). ### TODO - [x] **Q:** what about on Windows? **A:** we'll treat any exit code outside of 1 - 127 as "crashed", and we'll do the same on unix. - [x] test all permutations of actual vs expected **Done:** See #143002 (comment). - [x] Handle targets without unwind support - [x] Add `run-fail-or-crash` for some sanitizer tests ### Zulip discussion See https://rust-lang.zulipchat.com/#narrow/channel/122651-general/topic/compiletest.3A.20terminate.20by.20signal.20vs.20exit.20with.20error/with/525611235 try-job: aarch64-apple try-job: x86_64-msvc-1 try-job: x86_64-gnu try-job: dist-i586-gnu-i586-i686-musl try-job: test-various
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
💔 Test failed - checks-actions |
bors
added
S-waiting-on-review
|
Looks like @rustbot author |
rustbot
added
S-waiting-on-author
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
And introduce two new directives for ui tests:
run-crashrun-fail-or-crashNormally a
run-failui test like tests that panic shall not be terminated by a signal likeSIGABRT. So begin having that as a hard requirement.Some of our current tests do terminate by a signal/crash however. Introduce and use
run-crashfor those tests. Note that Windows crashes are not handled by signals but by certain high bits set on the process exit code. Example exit code for crash on Windows:0xc000001d(STATUS_ILLEGAL_INSTRUCTION). Because of this, we define "crash" on all platforms as "not exit with success and not exit with a regular failure code in the range 1..=127".Some tests behave differently on different targets:
run-failtests for such targets.run-fail-or-crashfor such tests.This adds further (cc #142304, #142886) protection against the regression in #123733 since that bug also manifested as
SIGABRTintests/ui/panics/panic-main.rs(shown asAborted (core dumped)in the logs attached to that issue, and I have also been able to reproduce this locally).TODO
A: we'll treat any exit code outside of 1 - 127 as "crashed", and we'll do the same on unix.
Done: See tests: Require
run-failui tests to have an exit code (SIGABRTnot ok) #143002 (comment).run-fail-or-crashfor some sanitizer testsZulip discussion
See https://rust-lang.zulipchat.com/#narrow/channel/122651-general/topic/compiletest.3A.20terminate.20by.20signal.20vs.20exit.20with.20error/with/525611235
try-job: aarch64-apple
try-job: x86_64-msvc-1
try-job: x86_64-gnu
try-job: dist-i586-gnu-i586-i686-musl
try-job: test-various